Skip to content

Conversation

@adameska
Copy link
Contributor

@adameska adameska commented Nov 3, 2025

This makes it easier to spot warnings/errors and identify new log lines

@feloy
Copy link
Contributor

feloy commented Nov 12, 2025

cc @Firewall @slemeur (screenshot and details are available at #417)

@vancura
Copy link
Member

vancura commented Nov 13, 2025

Can we work on the colors, please? They're not contrasty enough.

ERROR: '\u001b[31m',
FATAL: '\u001b[31;1m', // bright red
TRACE: '\u001b[35m', // magenta
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these the colors used, or are we loading them from elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the ANSI colors supported by the terminal. If we want to change the color to be rendered at the end, we may want to change them in the theme of the terminal. I think it could be part of another PR

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 let's create a separate issue for that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, sure, let's work on it in another ticket.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feloy I want to file the issue, but I am not sure which repo it should land in. Here or in PD, please?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slemeur
Copy link

slemeur commented Nov 13, 2025

Thanks @adameska for your contribution and the suggestion. I really do like the idea and think it'll help the experience.
+1 on my side for the PR

Signed-off-by: David Hettinger <[email protected]>
@adameska adameska requested review from a team and benoitf as code owners November 13, 2025 14:55
@adameska adameska requested review from dgolovin and feloy and removed request for a team November 13, 2025 14:55
@feloy
Copy link
Contributor

feloy commented Nov 13, 2025

pnpm format:check is failing on CI, you will need to run pnpm format:fix to fix
I can see there is also a few fixes to do to make pnpm lint:check pass

@adameska
Copy link
Contributor Author

pnpm format:check is failing on CI, you will need to run pnpm format:fix to fix I can see there is also a few fixes to do to make pnpm lint:check pass

Thank you! I saw something about prettier and was trying to run that to fix it but that wasn't taking...

This file contains a class for colorizing JSON strings with customizable ANSI color schemes for different JSON elements.

Signed-off-by: David Hettinger <[email protected]>
Add unit tests for JsonColorizer to validate colorization of various JSON structures, including braces, brackets, numbers, booleans, null values, and strings with custom color schemes.

Signed-off-by: David Hettinger <[email protected]>
Updated log level color mappings and enhanced the log level detection regex to support additional formats.

Signed-off-by: David Hettinger <[email protected]>
@adameska
Copy link
Contributor Author

adameska commented Nov 14, 2025

Ok, i fixed the linting issues. We've also been migrating over to structured logging which looks horrible in podman so I attempted to make those look better... once this PR gets complete if advisable i wouldn't mind adding a sidebar when structured logging was detected to show the values in a nice table.
image
image

@feloy
Copy link
Contributor

feloy commented Nov 14, 2025

Ok, i fixed the linting issues. We've also been migrating over to structured logging which looks horrible in podman so I attempted to make those look better... once this PR gets complete if advisable i wouldn't mind adding a sidebar when structured logging was detected to show the values in a nice table. image image

The feature is interesting, but I'm afraid it will be very cpu-consuming if it enabled for all containers. I think it would be interesting for the user to select which format to use for each container. @slemeur WDYT?

@adameska
Copy link
Contributor Author

Ok, i fixed the linting issues. We've also been migrating over to structured logging which looks horrible in podman so I attempted to make those look better... once this PR gets complete if advisable i wouldn't mind adding a sidebar when structured logging was detected to show the values in a nice table. image image

The feature is interesting, but I'm afraid it will be very cpu-consuming if it enabled for all containers. I think it would be interesting for the user to select which format to use for each container. @slemeur WDYT?

I didn't consider the cpu much. I feel like it should be ok but I could just parse the first 10 rows and if 9 of them have recognizable json then enable it if it seems fast enough when I test it with a 10k long file. I don't like the idea of having users enter their logging format since a lot of our applications use different ones.

@benoitf
Copy link
Contributor

benoitf commented Nov 15, 2025

hello, maybe bring an option to enable this feature. So it's an option people can turn on or off. The most difficult part would be to choose the default behavior.

@adameska
Copy link
Contributor Author

hello, maybe bring an option to enable this feature. So it's an option people can turn on or off. The most difficult part would be to choose the default behavior.

I have the JSON colorization not wired up by default (just added all the code for it for now). An option seems like a good idea although not sure how to add that outside of the toolbar i added in the other PR. I think for now this only coloring log level is minimal enough that it shouldn't offend people i hope...

I did locally enable coloring all json for a container with a ton of json log lines and it loaded up instantly for me so i'm not at all concerned about performance personally... but something you guys could test and let me know.

Let me know how you want to proceed. IMO just completing this PR as is (only coloring log level for now but adding the code for JSON coloring to be wired up from options) should be safe enough and hopefully provide value as is.

image

@feloy
Copy link
Contributor

feloy commented Nov 18, 2025

In this PR, I would suggest to include the code for the non-JSON colorization only, and keep the JSON-related code for another PR, when it can be activated.

You may have an option at the extension level, so the user could choose between text and JSON colorization, for all containers. It is not very realistic, as all containers are not always using the same log format, but that would be a first step to be able to test the JSON one.
You can see how to declare such configuration in the podman extension for example: https://github.com/podman-desktop/podman-desktop/blob/main/extensions/podman/packages/extension/package.json#L49
and access the value using the extension api: https://github.com/podman-desktop/podman-desktop/blob/main/packages/extension-api/src/extension-api.d.ts#L1219

@adameska
Copy link
Contributor Author

In this PR, I would suggest to include the code for the non-JSON colorization only, and keep the JSON-related code for another PR, when it can be activated.

You may have an option at the extension level, so the user could choose between text and JSON colorization, for all containers. It is not very realistic, as all containers are not always using the same log format, but that would be a first step to be able to test the JSON one. You can see how to declare such configuration in the podman extension for example: https://github.com/podman-desktop/podman-desktop/blob/main/extensions/podman/packages/extension/package.json#L49 and access the value using the extension api: https://github.com/podman-desktop/podman-desktop/blob/main/packages/extension-api/src/extension-api.d.ts#L1219

I'm going to push hard against users picking one or the other. It's easy to detect if a file is logged in json/structure logging format so we can detect and process if that's the case. IMO the only thing the user should be able to pick is a color theme potentially and colorizing logs at all on/off. I will put the Json coloring in a separate PR but that will be dependent on this PR

Removed tests for JSON colorization function.

Signed-off-by: David Hettinger <[email protected]>
Removed unused JSON colorizer and related color scheme.

Signed-off-by: David Hettinger <[email protected]>
Signed-off-by: David Hettinger <[email protected]>
@feloy
Copy link
Contributor

feloy commented Nov 18, 2025

Sorry, there was a problem on the pnpm-lock file. The problem should be fixed on main, You'll need to rebase so I can run the ci again

Copy link
Contributor

@feloy feloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see some characters which should not be displayed:

Image

Signed-off-by: Adameska <[email protected]>
@adameska
Copy link
Contributor Author

adameska commented Nov 19, 2025

I can see some characters which should not be displayed:

Image

glad you tested that case (mine have err) i had a typo. Should be fixed now! I also thought i deleted the json files but guess i never synced those. Hopefully this is good to go now.

Copy link
Contributor

@feloy feloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Great work, thanks!

@feloy feloy merged commit 43c1684 into podman-desktop:main Nov 21, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants